Update username in EIP-1319 #2550
Conversation
|
Hi! I'm a bot, and I wanted to automerge your PR, but couldn't because of the following issue(s):
|
|
ping @gnidan @iamdefinitelyahuman @pipermerriam for input/confirmation. Specifically if all looks well, could @gnidan approve this pr - since my email listed in the pr is not the one associated with my github, so I can't get this merged. |
|
I like it! Looks very clean. Thanks for pinging me on this. |
EIPS/eip-1319.md
Outdated
| `ethpm://defi.snakecharmers.eth/compound@2%402` | ||
|
|
||
| #### `compound` package, version `1.1.0`, `CErc20` abi | ||
| `ethpm://defi.snakecharmers.eth/compound@1.1.0/contract_types/CErc20/abi` |
There was a problem hiding this comment.
It would be nice if each of these was available as something that could be easily used as a test vector. They technically can in their current form, but it might be nice to have them available as YAML/JSON documents so that they are more easily portable.
EIPS/eip-1319.md
Outdated
| - If the package version contains any [url unsafe characters](https://www.werockyourweb.com/url-escape-characters/), they **must** be safely escaped | ||
| - Since semver is not strictly enforced by the ethpm spec, if the `package_version` is omitted from a uri, it is up to the tooling / framework to look up all available versions and ask the user which version they want to utilize. | ||
|
|
||
| #### `json_pointer` |
There was a problem hiding this comment.
This section should have at minimum a reference to the JSON-pointer specification number (assuming it is an RFC) and maybe a link/URI to the actual specification.
EIPS/eip-1319.md
Outdated
| - Optional | ||
| - String of the target package version | ||
| - If the package version contains any [url unsafe characters](https://www.werockyourweb.com/url-escape-characters/), they **must** be safely escaped | ||
| - Since semver is not strictly enforced by the ethpm spec, if the `package_version` is omitted from a uri, it is up to the tooling / framework to look up all available versions and ask the user which version they want to utilize. |
There was a problem hiding this comment.
This would probably be better to use the **should** language. Something like:
if the
package_versionis omitted from a uri, tooling should avoid guessing in the face of any ambiguity and present the user with a choice from the available versions.
EIPS/eip-1319.md
Outdated
|
|
||
| #### `registry_address` | ||
| - Required | ||
| - This **should** be an ENS name whenever possible, or a `0x`-prefixed, checksummed address. |
There was a problem hiding this comment.
This reads as if an ENS name is preferable to the other formats. Maybe reword as
This should be either an ENS name or a
0x-prefixed, checksummed address. ENS names are more suitable for cases where mutability of the underlying asset is acceptable and there is implicit trust in the owner of the name.0xprefixed addresses are more preferable in higher security cases to avoid needing to trust the controller of the name.
|
@njgheorghita is this still outstanding for merging? |
|
@axic Thanks for the reminder. It has all the necessary approvals afaik, it's just that the email address associated with my github account is not the one listed under my name as an author on this ERC. Would you be able to push the merge through? |
|
@njgheorghita do you want to change the author line to github username or another email so you can merge these draft changes? |
|
Can you also merge in master (or rebase)? We should check what the validator says now as this was creating quite a while ago. |
EIPS/eip-1319.md
Outdated
| @@ -166,6 +166,63 @@ release versions while allowing them to use any versioning schema they choose. | |||
|
|
|||
| Registries may offer more complex `read` APIs that manage requests for packages within a semver range or at `latest` etc. This EIP is agnostic about how tooling or registries might implement these. It recommends that registries implement [EIP 165](https://github.com/ethereum/EIPs/blob/master/EIPS/eip-165.md) and avail themselves of resources to publish more complex interfaces such as [EIP 926](https://github.com/ethereum/EIPs/blob/master/EIPS/eip-926.md). | |||
|
|
|||
| ## EthPM URIs | |||
There was a problem hiding this comment.
I'm not fully sure the best way is to include both the URI format and everything else in a single EIP. Would suggest for consideration to make this a separate EIP depending on this one.
njgheorghita
force-pushed
the
ethpm-uri-definition
branch
4 times, most recently
from
4b4b502 to
35e92b5
Compare
|
Should probably make this EIP require |
|
Though, it currently only references it casually in an example, without making any other mention anywhere in the specification of it. Maybe we can just remove that one example and merge this (which ends up being just an author update)? |
njgheorghita
force-pushed
the
ethpm-uri-definition
branch
from
35e92b5 to
0b6ccc7
Compare
|
@MicahZoltu Agreed, made the change, thanks! |
Added the specification for ethPM uris. They've been an effective solution for handling packages so far - and are already implemented in the
ethpm-cliandBrownie, and are also outlined in the documentation.